Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redefine objective sense as a proper IntEnum #3224

Merged
merged 15 commits into from
Apr 24, 2024

Conversation

jsiirola
Copy link
Member

@jsiirola jsiirola commented Apr 3, 2024

Fixes #993 .

Summary/Motivation:

There have been some longstanding inconsistencies regarding how we manage the definition of "objective sense" in Pyomo. The results object uses an Enum with 3 values (minimize, maximize, and unknown). In the modeling side, we make heavy use of two named constants (minimize and maximize). These constants are just ints (1 and -1, respectively), and those constants are defined in pyomo.core.kernel.objective (causing one of the few inter-linkages between kernel and the AML).

This PR defines a new Enum (pyomo.common.enums.ObjectiveSense), and redefines the minimize and maximize constants to be the members of that Enum. To preserve consistency / backwards compatibility, ObjectiveSense is an IntEnum, so the old constants / old usage does not need to change.

ProblemSense is now an extension of ObjectiveSense that also admits unknown. Unfortunately, due to the way that the old solvers are structured, it is not (easy) to remove the use of unknown when processing results from the old solver interfaces. The compromise here is the development of ExtendedEnumType, which allows us to define extended (or derived) Enums that combine the enums from another class with additional locally-defined members - so the minimize and maximize members in ProblemSense are actually members of ObjectiveSense.

Changes proposed in this PR:

  • Create ExtendedEnumType metaclass to support extended/derived Enums
  • Add ObjectiveSense enum (with minimize and maximize members)
  • Redefine ProblemSense to be an extension of ObjectiveSense
  • Remove references to ProblemSense from almost all of Pyomo
  • Fix an error in processing the lower / upper bound in the SCIPAMPL interface

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@jsiirola jsiirola changed the title Redefine objective senseas a proper IntEnum Redefine objective sense as a proper IntEnum Apr 3, 2024
Copy link
Contributor

@emma58 emma58 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! I have one question about the SCIP change below--just wondering if you could fall victim to numerical issues.

pyomo/opt/results/problem.py Outdated Show resolved Hide resolved
if results.problem.sense == ProblemSense.minimize:
if results.solver.primal_bound < results.solver.dual_bound:
results.problem.lower_bound = results.solver.primal_bound
results.problem.upper_bound = results.solver.dual_bound
else:
results.problem.lower_bound = results.solver.dual_bound
results.problem.upper_bound = results.solver.primal_bound
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this go wrong if your bounds cross by a little bit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - you probably can. This will be easily fixable in the new solver interfaces (where the results parser can have easier access to the original model and won't have to guess about the sense), but there isn't an obvious better solution here (unless the scip output log prints something about the sense - but I really don't want to put more dependence on the parsing of output logs).

@mrmundt mrmundt merged commit 9608e0e into Pyomo:main Apr 24, 2024
33 checks passed
@jsiirola jsiirola deleted the objective-sense branch April 26, 2024 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conflicting definitions of minimize
4 participants